sql: Add INET column type and IPAddr datum #18171

lgo · 2017-09-01T21:57:02Z

This is an implementation for the INET postgres column type, found at Network Address Types. Related to #6981. At the moment this PR does not contain functions or operators found on Network Address Functions and Operators, but this is being done in another PR.

I've added pkg/util/ipaddr/ipaddr.go to provide utility functions as a wrapper around net.IPNet. This includes marshaling to/from binary and parsing INET addresses (and in the future parsing CIDR).

The parsing functions were made as Go's net.ParseIP and net.ParseCIDR don't suffice for postgres compatibility. Postgres INET values can have partial octets and a trailing .. For example these values are valid,

192.168/16
196.168./16
192.168.0.1.

Another key factor is that under Go an IPv4-mapped IPv6 address is equal to it's mapped IPv4 address, while in postgres this is not the case. This presents a problem, so we need to differentiate between the families explicitly. This is also important for ordering properties seen below.

Go

ip1 := net.ParseIP("127.0.0.1")
ip2 := net.ParseIP("::ffff:127.0.0.1")
ip1.Equal(ip2)
> true

Postgres

select '::ffff:127.0.0.1'::inet = '127.0.0.1'::inet;
----------
 f

select * from test order by ip;
-----------------------
190.0.0.0/1
192.168.0.0/1
192.168.0.1/1
192.168.0.1/30
192.168.0.1
192.168.0.2
::ffff:192.168.0.1/29
::ffff:192.168.0.1/30
::
::1
::f
::ffff:192.168.0.1
f:f:f:f:f:f:f:f

Encoding

The encoding scheme is

Family	Mask	IP
1-byte	1-byte	{4,16}-byte

The family can be 4 or 6, and the mask can be 1-32 for IPv4, or 1-128 for IPv6.

If the IP is IPv6, it will be 16-bytes. This falls close to Postgres, where they will use 1 additional byte in total[1].

[1]: I've yet to figure out why they have one extra byte. See their Network Address Types

cockroach-teamcity · 2017-09-01T21:57:09Z

This change is

benesch · 2017-09-01T22:44:45Z

Re [1]: https://github.com/postgres/postgres/blob/c7b8998ebbf310a156aa38022555a24d98fdbfb4/src/include/utils/inet.h#L42-L51

knz · 2017-09-04T09:14:20Z

You also need to test the comparison of '192.168.0.2/24'::inet < '192.168.0.1/25'::inet. It's not trivial and motivates why the mask should be encoded before the address for ordering.

lgo · 2017-09-05T12:39:28Z

Yep, those comparisons are tested at a few levels but there aren't too many in the logic tests (or they are incorrect). I haven't made the logic tests thorough because storing and retrieving the values isn't working.

lgo · 2017-09-05T12:40:52Z

Also a follow up @knz. After adding the IPNet datum, how easy will it be to add the CIDR column type to use this also? I noticed the issue #8318 which talks about "compound types". Would the lack of any true compound type make it more difficult to add CIDR and re-use this code?

knz · 2017-09-05T12:44:42Z

CIDR has nothing to do with compound types. I think what you should be looking at to find something to reuse is the difference between DName and DString.

knz · 2017-09-05T12:45:02Z

Also, talk to Jordan.

lgo · 2017-09-05T17:21:20Z

@jordanlewis @knz please take a look :). This PR is done and working 🎉 . I've left comments in for places to add the CIDR column, which I can remove before merging if desired as I'll defer that to another PR.

A few questions I had:

Where can I find specs on pgwire? I was uncertain about the pgwire encoding/decoding, so that's noted in the PR and currently stubbed out with the internal binary encode/decode.
What is Datum.Prev and Datum.Next used for? Do you think they will be needed for INET/CIDR columns? Some basic logic is there but commented out as the definition is ambiguous when you consider different family and mask values.

benesch · 2017-09-05T17:27:11Z

@LEGO Postgres's wire protocol is documented in Chapter 51 of the PostgreSQL docs. The morpheme "pgwire" is a Cockroachism, which is why Googling it is so useless. The rest of the internet calls pgwire the "Postgres protocol" or similar.

All that said, the Postgres docs will usually just direct you to the source to determine the encoding for a particular datum.

The text representation of values is whatever strings are produced and accepted by the input/output conversion functions for the particular data type. In the transmitted representation, there is no trailing null character; the frontend must add one to received values if it wants to process them as C strings. (The text format does not allow embedded nulls, by the way.)

Binary representations for integers use network byte order (most significant byte first). For other data types consult the documentation or source code to learn about the binary representation. Keep in mind that binary representations for complex data types might change across server versions; the text format is usually the more portable choice.

https://www.postgresql.org/docs/current/static/protocol-overview.html

I think the file you want to look at here is https://github.com/postgres/postgres/blob/81c5e46c490e2426db243eada186995da5bb0ba7/src/backend/utils/adt/network.c.

lgo · 2017-09-18T16:54:56Z

Was that the pg_catalog failure? It was the type length changed during the net.IP to uint128 change, all of the other logic tests are now passing :)

Review status: 44 of 48 files reviewed at latest revision, 5 unresolved discussions.

c-deps/rocksdb, line 1 at r9 (raw file):

Previously, knz (kena) wrote…

I don't see any change.

Okay, got it after the gitmodules were being tricky :)

(It keeps auto-changing even when it was reset, but the proper one was getting staged and the other wasn't but I kept adding it.

pkg/cli/dump_test.go, line 61 at r11 (raw file):

Previously, knz (kena) wrote…

duplicate column name

Done.

Comments from Reviewable

knz · 2017-09-18T17:11:26Z

nice work!

Reviewed 4 of 4 files at r12.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks pending.

Comments from Reviewable

knz · 2017-09-18T17:13:56Z

you forgot to run build/builder.sh make generate

Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks pending.

Comments from Reviewable

lgo · 2017-09-18T17:37:29Z

👍 thanks. Also realized where the rowsort fix is needed. Churning through the final test failures then it should be good :)

Review status: 47 of 48 files reviewed at latest revision, 3 unresolved discussions.

Comments from Reviewable

lgo · 2017-09-18T20:23:21Z

Ah something I left as a TODO, currently, the encoding is using 16-bytes for an IPv4 address. I'm going to swap that back to being more optimal for IPv4. I dropped that when I first made the switch to uint128.

I reckon it's no easy matter to change this after the initial feature.

Review status: 42 of 48 files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

Comments from Reviewable

knz · 2017-09-18T20:27:59Z

Okay let's do that. Let me know when it's ready.

lgo · 2017-09-18T20:51:15Z

👍 just pushed it up

Review status: 37 of 48 files reviewed at latest revision, 3 unresolved discussions, some commit checks pending.

Comments from Reviewable

knz · 2017-09-18T20:58:12Z

Reviewed 2 of 21 files at r5, 11 of 11 files at r13.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks pending.

pkg/util/ipaddr/ipaddr.go, line 65 at r13 (raw file):

const IPv6size = net.IPv6len + 2

// IPv4mappedIPv6prefix is the byte prefix for IPv4-mapped IPv6

Period missing at end of sentence.

pkg/util/ipaddr/ipaddr.go, line 73 at r13 (raw file):

	appendTo = append(appendTo, byte(ipAddr.Family), ipAddr.Mask)
	if ipAddr.Family == IPv4family {
		var tmp [4]byte

appendTo = append(appendTo, 0, 0, 0, 0)
binary.BigEndian.PutUint32(tmp[len(tmp)-5:len(tmp)], ...)

pkg/util/ipaddr/ipaddr.go, line 78 at r13 (raw file):

	} else {
		var tmp [16]byte
		binary.BigEndian.PutUint64(tmp[:8], ipAddr.Addr.Hi)

ditto

no need to copy the data twice.

Comments from Reviewable

lgo · 2017-09-19T00:40:04Z

Review status: 47 of 48 files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.

pkg/util/ipaddr/ipaddr.go, line 65 at r13 (raw file):

Previously, knz (kena) wrote…

Period missing at end of sentence.

Done.

pkg/util/ipaddr/ipaddr.go, line 73 at r13 (raw file):

binary.BigEndian.PutUint32(tmp[len(tmp)-5:len(tmp)], ...)
Right, so this ensures that the slice has enough space immediately(and/or allocates it)? I was looking into pre-allocating space in the slice via. a new make and copy but that was obviously not ideal, so this is good :)

pkg/util/ipaddr/ipaddr.go, line 78 at r13 (raw file):

Previously, knz (kena) wrote…

ditto

no need to copy the data twice.

Done.

Comments from Reviewable

knz · 2017-09-19T00:46:30Z

thanks 🍻

Reviewed 1 of 21 files at r5, 1 of 1 files at r14.
Review status: all files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

pkg/util/ipaddr/ipaddr.go, line 73 at r13 (raw file):

Previously, lego (Joey Pereira) wrote…

binary.BigEndian.PutUint32(tmp[len(tmp)-5:len(tmp)], ...)
Right, so this ensures that the slice has enough space immediately(and/or allocates it)? I was looking into pre-allocating space in the slice via. a new make and copy but that was obviously not ideal, so this is good :)

Yes append of these zeros will allocate the extra space.

Comments from Reviewable

This introduces upport for the PostgreSQL column type, INET, which can be found at https://www.postgresql.org/docs/9.6/static/datatype-net-types.html.

lgo requested review from jordanlewis, m-schneider and a team September 1, 2017 21:57

lgo changed the title ~~Add INET column type and IPNet datum~~ sql: Add INET column type and IPNet datum Sep 1, 2017

lgo force-pushed the joey/ip-coltype branch from 51ee5c5 to 442e31f Compare September 1, 2017 22:04

lgo force-pushed the joey/ip-coltype branch 2 times, most recently from 4e253bc to c45b288 Compare September 2, 2017 05:06

lgo force-pushed the joey/ip-coltype branch 6 times, most recently from 8809939 to e27eed7 Compare September 5, 2017 17:00

lgo requested a review from knz September 5, 2017 17:00

lgo force-pushed the joey/ip-coltype branch from e27eed7 to 89a83db Compare September 5, 2017 17:21

lgo force-pushed the joey/ip-coltype branch 3 times, most recently from 6cfefd7 to f77bff3 Compare September 18, 2017 16:53

lgo force-pushed the joey/ip-coltype branch from f77bff3 to e827dfa Compare September 18, 2017 17:04

lgo force-pushed the joey/ip-coltype branch from e827dfa to 6ca8545 Compare September 18, 2017 17:30

lgo force-pushed the joey/ip-coltype branch 3 times, most recently from 81ebc8f to 3bd6de6 Compare September 18, 2017 18:43

lgo force-pushed the joey/ip-coltype branch from 3bd6de6 to d183262 Compare September 18, 2017 20:46

lgo force-pushed the joey/ip-coltype branch 2 times, most recently from 4347fc2 to 9039c48 Compare September 18, 2017 21:25

sql: Add INET column type and IPAddr datum

060e60b

This introduces upport for the PostgreSQL column type, INET, which can be found at https://www.postgresql.org/docs/9.6/static/datatype-net-types.html.

lgo force-pushed the joey/ip-coltype branch from 9039c48 to 060e60b Compare September 19, 2017 01:06

lgo merged commit 028bbd8 into cockroachdb:master Sep 19, 2017

lgo changed the title ~~sql: Add INET column type and IPNet datum~~ sql: Add INET column type and IPAddr datum Sep 20, 2017

lgo added the first-pr Use to mark the first PR sent by a contributor / team member. Reviewers should be mindful of this. label Sep 20, 2017

lgo deleted the joey/ip-coltype branch September 25, 2017 19:16

lgo mentioned this pull request Oct 17, 2017

sql: Support INET column type operators #18847

Closed

jseldess mentioned this pull request Oct 30, 2017

inet data types cockroachdb/docs#2016

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: Add INET column type and IPAddr datum #18171

sql: Add INET column type and IPAddr datum #18171

lgo commented Sep 1, 2017 •

edited

Loading

cockroach-teamcity commented Sep 1, 2017

benesch commented Sep 1, 2017

knz commented Sep 4, 2017

lgo commented Sep 5, 2017

lgo commented Sep 5, 2017

knz commented Sep 5, 2017

knz commented Sep 5, 2017

lgo commented Sep 5, 2017 •

edited

Loading

benesch commented Sep 5, 2017 •

edited

Loading

lgo commented Sep 18, 2017

knz commented Sep 18, 2017

knz commented Sep 18, 2017

lgo commented Sep 18, 2017

lgo commented Sep 18, 2017

knz commented Sep 18, 2017

lgo commented Sep 18, 2017

knz commented Sep 18, 2017

lgo commented Sep 19, 2017

knz commented Sep 19, 2017

sql: Add INET column type and IPAddr datum #18171

sql: Add INET column type and IPAddr datum #18171

Conversation

lgo commented Sep 1, 2017 • edited Loading

Go

Postgres

Encoding

cockroach-teamcity commented Sep 1, 2017

benesch commented Sep 1, 2017

knz commented Sep 4, 2017

lgo commented Sep 5, 2017

lgo commented Sep 5, 2017

knz commented Sep 5, 2017

knz commented Sep 5, 2017

lgo commented Sep 5, 2017 • edited Loading

benesch commented Sep 5, 2017 • edited Loading

lgo commented Sep 18, 2017

knz commented Sep 18, 2017

knz commented Sep 18, 2017

lgo commented Sep 18, 2017

lgo commented Sep 18, 2017

knz commented Sep 18, 2017

lgo commented Sep 18, 2017

knz commented Sep 18, 2017

lgo commented Sep 19, 2017

knz commented Sep 19, 2017

lgo commented Sep 1, 2017 •

edited

Loading

lgo commented Sep 5, 2017 •

edited

Loading

benesch commented Sep 5, 2017 •

edited

Loading